Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TINY-11177: Vastly improve remote testing #145

Merged
merged 64 commits into from
Jan 27, 2025
Merged

Conversation

TheSpyder
Copy link
Member

@TheSpyder TheSpyder commented Aug 16, 2024

Related Ticket: TINY-11177

Description of Changes:

  • Remote testing was slow because bedrock sent session POST requests to /start before each it block, and then again to /results after each it block. It would wait for these requests to finish before continuing.
  • /start is now sent at startup only.
  • /results is now only sent for failure and skip, or every 30 seconds if tests are passing happily.
  • The bedrock UI doesn't block on these status requests anymore, it just throws them into an array and uses Promise.all() at the very end to wait for them.
  • Unfortunately the bedrock CLI console HUD depended on receiving these status updates, so I had to adjust it to account for the missing reports.
  • The bedrock HTML page now has a fake icon to remove the favicon.ico request which probably also slowed things down.
  • A proper keep-alive header is now set, instead of using the default 5 seconds which was causing occasional 502 Bad Gateway errors.
  • Reverted TINY-10708: Suppress bedrock logs in remote testing #142 which was designed to solve this problem but server-side was the wrong answer.

Pre-checks:

  • Changelog entry added
  • package.json versions have not been changed (done by Lerna on release)
  • Tests have been added (if applicable)

Before merging:

  • Ensure internal dependencies are on appropriate versions
    • For stable releases, all dependencies must be stable
    • For release candidates, all dependencies must be release candidates or stable

@TheSpyder TheSpyder requested a review from tjdett August 16, 2024 06:37
@TheSpyder TheSpyder requested a review from a team as a code owner August 16, 2024 06:37
Copy link
Member

@spocke spocke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this affect junit reporting would the in flight requests make them in random order or would things be skipped.

@TheSpyder
Copy link
Member Author

I posted in slack that this destroys JUnit reporting. Ideas for how to fix:
https://cksource.slack.com/archives/C04KXE6QUTX/p1724022794632699?thread_ts=1723766938.410059&cid=C04KXE6QUTX

@TheSpyder TheSpyder marked this pull request as draft August 19, 2024 00:05
Jenkinsfile Outdated Show resolved Hide resolved
modules/runner/src/main/ts/reporter/Reporter.ts Outdated Show resolved Hide resolved
@ltrouton ltrouton self-requested a review September 17, 2024 23:23
TheSpyder and others added 18 commits December 4, 2024 11:13
…it for the report AJAX requests to complete before moving on to the next test.
…it would be misleading if they were treated as passing.
…ts. Manual mode works, auto still needs some work.
…to the end of the test run are now waited for correctly.
@TheSpyder TheSpyder marked this pull request as ready for review January 21, 2025 05:22
@TheSpyder TheSpyder requested a review from spocke January 21, 2025 05:23
Copy link
Contributor

@ltrouton ltrouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an expert on bedrock so don't know everything I am looking at but this looks like it will be a big improvement 👍

modules/runner/src/main/ts/reporter/Reporter.ts Outdated Show resolved Hide resolved
Copy link
Member

@spocke spocke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, lots of nice improvements beyond the perf things. 👍

modules/runner/src/main/ts/runner/Runner.ts Outdated Show resolved Hide resolved
modules/runner/src/main/ts/runner/TestRun.ts Show resolved Hide resolved
modules/runner/src/main/ts/runner/TestRun.ts Outdated Show resolved Hide resolved
modules/server/src/main/ts/bedrock/server/Apis.ts Outdated Show resolved Hide resolved
modules/server/src/main/ts/bedrock/server/EffectUtils.ts Outdated Show resolved Hide resolved
modules/server/tasks/bedrock.js Show resolved Hide resolved
@TheSpyder
Copy link
Member Author

Resolved all except one which I'll deal with on Monday and then release this 👍

…o simplify the code that calls it.

Removed unnecessary export declarations, which means we no longer need a safety-net `checkSiblings` call in `runSuites` which will by definition never pass the root test suite condition.
@TheSpyder
Copy link
Member Author

I'm glad to finally finish this one ☺️

@TheSpyder TheSpyder merged commit 18ed8c5 into master Jan 27, 2025
5 checks passed
@TheSpyder TheSpyder deleted the feature/TINY-11177 branch January 27, 2025 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants